Conversation
|
I've assigned @tnull as a reviewer! |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
4 times, most recently
from
29f47f3 to
264aa7f
Compare
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 13th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
264aa7f to
493dd9a
Compare
|
🔔 14th Reminder Hey @tnull! This PR has been waiting for your review. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
a30cbfb to
1e7bdbc
Compare
|
🔔 15th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 16th Reminder Hey @tnull! This PR has been waiting for your review. |
There was a problem hiding this comment.
Thanks for looking into this and excuse the delay here!
I did a first pass, generally this looks already pretty good, but it is a huge PR and in some areas could be simplified. For instance, we should drop the generic Retry logic as the concrete implementations would already implement that if they need it. Likewise, we shouldn't fallback to the backup for now as it's really only meant as disaster recovery (KVStore caching is out-of-scope for this PR now, even though we might want to explore that soon, too). There is also no need to replicate the write-ordering locks in TierStore, but we'll need them in ForeignKVStoreAdapter (which might be mergeable with DynStore?).
Generally, if you find opportunities to reduce the size of the changeset here it would be appreciated. It would also be cool if you could try to break up the PR into more feature commits, but feel free to leave as is if not.
src/io/tier_store.rs
Outdated
|
|
||
| /// Configuration for exponential backoff retry behavior. | ||
| #[derive(Debug, Copy, Clone)] | ||
| pub struct RetryConfig { |
There was a problem hiding this comment.
Retrying is pretty specific to the particular KVStore implementation. I don't think we should expose retrying params on top of what we already do for implementations internally.
src/io/tier_store.rs
Outdated
| } | ||
|
|
||
| pub struct TierStoreInner { | ||
| /// For remote data. |
bindings/ldk_node.udl
Outdated
| [Throws=BuildError] | ||
| Node build_with_vss_store_and_header_provider(NodeEntropy node_entropy, string vss_url, string store_id, VssHeaderProvider header_provider); | ||
| [Throws=BuildError] | ||
| Node build_with_tier_store(NodeEntropy node_entropy, DynStore primary_store); |
There was a problem hiding this comment.
Can we now also expose Builder::build_with_store?
src/ffi/types.rs
Outdated
| Self { inner: Arc::new(adapter) } | ||
| } | ||
|
|
||
| pub fn from_ldk_store(store: Arc<dyn LdkSyncAndAsyncKVStore + Send + Sync>) -> Arc<Self> { |
There was a problem hiding this comment.
If we rather make this an impl From<Arc<dyn LdkSyncAndAsyncKVStore + Send + Sync>> for Arc .., we can drop the wrap_storemacro and just use.into()` instead.
src/builder.rs
Outdated
| } | ||
|
|
||
| let store = wrap_store!(Arc::new(tier_store)); | ||
| self.build_with_store(node_entropy, store) |
There was a problem hiding this comment.
I think we need to use build_with_store_internal here to make sure we're using the same Runtime etc.
src/io/tier_store.rs
Outdated
| Arc::clone(&outer_lock.entry(locking_key).or_default()) | ||
| } | ||
|
|
||
| async fn execute_locked_write< |
There was a problem hiding this comment.
I'm confused, why are we replicating all the write-ordering logic here? Given the implementations are required to fullfill LDK's requirements already, can't we just call the inner write and be done with it?
| ) -> Pin<Box<dyn Future<Output = Result<(), lightning::io::Error>> + Send>> { | ||
| let inner = self.inner.clone(); | ||
|
|
||
| let primary_namespace = primary_namespace.to_string(); |
There was a problem hiding this comment.
I think here (and in remove) we need to add the write-ordering logic to ensure we follow LDK's KVStore::write requirements.
src/types.rs
Outdated
| /// A type alias for [`SyncAndAsyncKVStore`] with `Sync`/`Send` markers; | ||
| pub type DynStore = dyn SyncAndAsyncKVStore + Sync + Send; | ||
| #[cfg(feature = "uniffi")] | ||
| pub(crate) use crate::DynStore; |
There was a problem hiding this comment.
This is odd. Why do we need this?
src/io/test_utils.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub struct DelayedStore { |
There was a problem hiding this comment.
Not quite sure what coverage we gain with DelayedStore? IMO, we might be better off dropping all this boilerplate.
src/ffi/types.rs
Outdated
| ) -> Result<Vec<String>, IOError>; | ||
| } | ||
|
|
||
| pub struct ForeignKVStoreAdapter { |
There was a problem hiding this comment.
Hmm, any reason this needs to be separate from DynStore? Couldn't we merge them?
| } | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
There was a problem hiding this comment.
Probably not worth taking a dependency just to desugar
impl Future<Output = Result<(), IOError>> + Send + Sync + 'a.
Well, it's 'the official'/supported way to do async traits with Uniffi: https://mozilla.github.io/uniffi-rs/latest/futures.html#exporting-async-trait-methods
There was a problem hiding this comment.
Does uniffi require that in some way? Its just a comically-overkill way to sugar impl Future which is kinda nuts...
There was a problem hiding this comment.
Oh, worse, it looks like async_trait desugars to the Pin<Box<dyn ...>> version...which is dumb but at least for uniffi it shouldn't matter cause we'd have to do that anyway.
There was a problem hiding this comment.
Unfortunately this will need a substantial rebase now that #696 landed, sorry for that!
Besides some tedious rebase work, we now have DynStoreWrapper which can be used on the ffi and the non-ffi side. I do wonder if we can actually just merge that wrapper with the TierStore, as well as the ForeignKVStoreAdapter/DynStore. Seems all these wrapper structs may not be needed and we could just get away with one trait and one wrapper struct, mostly?
|
Probably, indeed. To avoid exposing the wrappers in Rust we'll have to put them in a module which is conditionally pub. |
|
@enigbe Do you still intend to go ahead with this PR? I'm afraid the number of conflicts with the main branch is rather substantial by now unfortunately. :/ |
Yes I do. Please excuse the delay. |
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
5 times, most recently
from
b5e980f to
67d47c2
Compare
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
2 times, most recently
from
95285b0 to
4b2d345
Compare
Introduces TierStore, a KVStore implementation that manages data across three storage layers: - Primary: Main data store for critical node data - Ephemeral: Secondary store for non-critical, easily-rebuildable data (e.g., network graph) with fast local access - Backup: Tertiary store for disaster recovery with async/lazy operations to avoid blocking primary store - Unit tests for TierStore core functionality
Adds TierStoreConfig and two configuration methods to NodeBuilder: - set_backup_store: Configure backup store for disaster recovery - set_ephemeral_store: Configure ephemeral store for non-critical data Modifies build_with_store to wrap the provided store in a TierStore, as the primary store, applying any configured ephemeral and backup stores. Note: Temporal dead code allowance will be removed in test commit.
Introduce FFI-safe abstractions and builder APIs to allow foreign language targets to configure custom backup and ephemeral stores when constructing nodes with a custom store. Major changes include: - Addition of FfiDynStoreTrait, an FFI-safe equivalent of DynStoreTrait, working around uniffi's lack of support for Pin<Box<T>> - Addition of FfiDynStore, a concrete wrapper for foreign language store implementations - Provision of FfiDynStoreTrait implementation for DynStoreWrapper to bridge native Rust stores to FFI layer (useful in testing) - Extension of ArcedNodeBuilder with methods for configuring backup and ephemeral stores - Exposure of build_with_store so foreign targets can build nodes with custom store implementations - Addition of build_node_with_store test helper to abstract uniffi-gated store wrapping at build_with_store call sites
- Add Rust integration test verifying correct routing to storage tiers - Add Python in-memory KV store and FFI test for tiered storage
Introduce FfiDynStoreInner with per-key write version locks that ensure write ordering and skip stale versions in both sync and async code paths. Test changes: - Unify tier store test helpers to use TestSyncStore for all tiers, replacing mixed SqliteStore/FilesystemStore/TestStore usage that caused test hangs due to TestStore's async write blocking - Split build_node_with_store into cfg-gated versions for uniffi vs non-uniffi feature flags
These were created to test that our backup store does not impact the primary store writes but the boilerplate appears too much for the functionality being tested.
- Restrict `TierStoreInner` visibility from `pub` to `pub(crate)` - Extract repeated ephemeral key matching into a standalone `is_ephemerally_cached_key` helper to DRY up `read_internal`, `write_internal`, and `remove_internal` - Replace `KVStoreSync::list` with async `KVStore::list` in `list_internal` to avoid blocking the async runtime
enigbe
force-pushed
the
2025-10-tiered-data-storage
branch
from
4b2d345 to
cba29a3
Compare
4 participants
What this PR does
In this PR we introduce
TierStore, a three-tiered (KVStore+KVStoreSync) implementation that manages data across three distinct storage layers based on criticality.Background
As we have moved towards supporting remote storage with
VssStore, we need to recognize that not all data has the same storage requirements. Currently, all data goes to a single store which creates some problems:This PR proposes tiered storage that provides granular control over where different data types are stored. The tiers include:
Additionally, we also permit the configuration of
Nodewith tiered storage allowing callers to:Nodewith a primary store.These configuration options also extend to our foreign interface, allowing bindings target to build the
Nodewith their own (KVStore+KVStoreSync) implementations. A sample Python implementation is provided and tested.Concerns
VssStorehas built-in retry logic. Wrapping it inTierStorecreates nested retries.KVStoreto the FFIRelated Issues